-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
multi: handle trade suspension messages. #269
Conversation
2f80765
to
3fa4fba
Compare
3fa4fba
to
512093c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've got an issue. The spec for suspension
is still based off of a simpler time when we had a single epoch duration for the entire DEX. That was changed a while back. The suspension
route can no longer send the epoch. It should probably be changed to either a timestamp, or a mapping of market->epoch or market->timestamp.
How do we reset the suspension? The docs do say
Users should expect to lose connection during suspension.
but I don't know if that's a strong enough guarantee that we could then hook into handleReconnect
to catch resumption of trade and perform the necessary actions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the server should send a market->epoch map in the suspension message. one message per market
But as far as this PR goes, I think we should put it on hold and first address the issue from the server side to be sure it is practical.
client/core/core.go
Outdated
return nil, fmt.Errorf("order placement suspended due to impending"+ | ||
" trade suspension for dex %s", dc.acct.url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth saying when this suspension is going to happen. (suspension epoch * epoch duration (ms)) We'd need to get this from the market map, but as @buck54321 pointed out, there will need to be a suspension epoch for each market.
I'll wrap up the server-side trade suspension PR in a day or two. After we're sure what info will come from the server we can resume this one. Thanks for hanging in there on this one. |
No worries, dcrpool work has been keeping me busy. |
Lines 710 to 717 in 73a3f78
|
Also, unless all of the markets are suspended, I don't think the client should expect a lost connection. |
noted, will be back at it this weekend. |
To clarify the current market suspend operation from the point of view of the client:
Also, I imagine the client will simply prevent submission of orders to suspended markets. Note that the config message also had this data, so clients that were offline when |
Noted, thank you. |
fe0b31d
to
a253026
Compare
Apologies, I edited the existing commit instead of adding a new one, will remember not to in the updates to come. @chappjc please provide some feedback when you can. Also let me know if a unified type that would hold the market suspension time and resumption time for a market would be preferable here, instead of using suspension and resumption message maps. |
Will have to review to give suggestion on the client implementation, but what to do really depends on server capabilities. I just put up a draft of the swapper suspend at #406 and it's looking feasible for the server to resend certain messages and to resume coin waiters. Will have a look at this tomorrow. |
ca109b0
to
5c58b22
Compare
a794c29
to
8bac022
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few concerns about concurrency. There's lots of mutexes.
8bac022
to
dc969a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple more.
aec60c2
to
aadd0e1
Compare
This adds the trade suspension handler to the client core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patience on this one. I felt we needed to get a better idea of what server is going to do before going too far down the suspend/resume road on the client side. I think this all makes sense for suspend. Just a few comments and questions. Will move on to the resume PR #442
aadd0e1
to
8a23ff7
Compare
8a23ff7
to
fc088db
Compare
I'm going to outline a plan for "connectivity robustness" in a new issue tomorrow because we need to look at the bigger picture to decide what needs to be done. "connectivity robustness" will cover any disruption, either planned or unplanned:
|
785d7dc
to
872e964
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still pondering if a final TradeSuspension message should also be sent from the server at the moment of market suspend, which would need handling client side of course, but I'm good with this as a start.
I would also like to call out the different scenarios mentioned in #269 (comment), namely what happens when a client connects when a suspension is already scheduled. Does the client create a pendingSuspension or flag a market as suspended based on the 'config'
response? If not, it will need to.
Once @buck54321's reviews are addressed and he approves, please rebase/resolve/squash. |
This adds the suspension handler to the client core.